Various `ScalarValue` numeric method fixes & refactors (especially decimal) by Jefffrey · Pull Request #23631 · apache/datafusion

Jefffrey · 2026-07-16T10:41:40Z

Which issue does this PR close?

N/A

Rationale for this change

I noticed there were some subtle errors with how decimals were handled in scalar values, and also opportunity to remove power calls in favour of precomputed constant tables. Also filling out some other missing support.

What changes are included in this PR?

I recommend looking at the commits as they are self contained with detailed messages for each.

Are these changes tested?

Yes

Are there any user-facing changes?

No

Since we already have access to these constant lookup tables, its more efficient to index into them rather than calculating via `checked_pow()` at runtime.

There's a side-effect of some tests failing so I removed them. I think this is fine since it doesn't make sense to have a `ScalarValue` method where we request a value of a certain type but can get a different value type out.

Seems like this was just a simple omission

For one/negative one, if the scale equals the precision then all values are to the right of the decimal point meaning we actually cannot represent a value of 1, so this could have caused some subtle errors. Similar for ten, we need at least 2 digits left of the decimal point, that is (p - s) >= 2, to represent a ten value.

Using macros here to generate these tables instead of manually enumerating the values into the code itself; for i256 its a bit complicated since it doesn't have native support so can't use direct multiplication as it doesn't have const support. Used Codex in this case, but added a unit test to ensure the constant values generated are correct.

I noticed we didn't have support for `Float16`, `Decimal32` and `Decimal64` here; instead of just adding them here, I figured it would be easier to plumb in the `ScalarValue` code to reduce duplication and ensure there's less opportunities for subtle omissions like this.

codecov-commenter · 2026-07-16T11:05:02Z

Codecov Report

❌ Patch coverage is 83.47826% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.68%. Comparing base (1e58928) to head (ce0f16b).

Files with missing lines	Patch %	Lines
datafusion/common/src/scalar/mod.rs	80.80%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23631      +/-   ##
==========================================
+ Coverage   80.67%   80.68%   +0.01%     
==========================================
  Files        1086     1087       +1     
  Lines      366800   366789      -11     
  Branches   366800   366789      -11     
==========================================
+ Hits       295912   295943      +31     
+ Misses      53260    53212      -48     
- Partials    17628    17634       +6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Jefffrey · 2026-07-16T11:24:37Z

msrv failure related to

Bump MSRV from 1.88.0 to 1.94.0 #23632

AdamGS · 2026-07-17T09:36:06Z

+    //
+    // This is mainly a shortcut for not needing to manually list out each value
+    // anyway.
+    let mut values = [i256::ONE; _];


I think this is basically const mul? I also ran into that recently elsewhere, so figured I'll make a PR - apache/arrow-rs#10363

this would be very helpful indeed 🙏

AdamGS · 2026-07-17T09:37:54Z

+    if let Expr::Literal(sv, _) = s
+        && sv.data_type().is_numeric()
+    {
+        // unwrap safe since numeric types always have a 0 value
+        sv == &ScalarValue::new_zero(&sv.data_type()).unwrap()
+    } else {
+        false
    }
 }

 pub fn is_one(s: &Expr) -> bool {
-    match s {
-        Expr::Literal(ScalarValue::Int8(Some(1)), _)
-        | Expr::Literal(ScalarValue::Int16(Some(1)), _)
-        | Expr::Literal(ScalarValue::Int32(Some(1)), _)
-        | Expr::Literal(ScalarValue::Int64(Some(1)), _)
-        | Expr::Literal(ScalarValue::UInt8(Some(1)), _)
-        | Expr::Literal(ScalarValue::UInt16(Some(1)), _)
-        | Expr::Literal(ScalarValue::UInt32(Some(1)), _)
-        | Expr::Literal(ScalarValue::UInt64(Some(1)), _) => true,
-        Expr::Literal(ScalarValue::Float32(Some(v)), _) if *v == 1. => true,
-        Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 1. => true,
-        Expr::Literal(ScalarValue::Decimal128(Some(v), _p, s), _) => {
-            *s >= 0
-                && POWS_OF_TEN
-                    .get(*s as usize)
-                    .map(|x| x == v)
-                    .unwrap_or_default()
-        }
-        Expr::Literal(ScalarValue::Decimal256(Some(v), _p, s), _) => {
-            *s >= 0
-                && match i256::from(10).checked_pow(*s as u32) {
-                    Some(res) => res == *v,
-                    None => false,
-                }
-        }
-        _ => false,
+    if let Expr::Literal(sv, _) = s
+        && sv.data_type().is_numeric()
+        // there are edge cases like negative scale decimals not being able to
+        // create a one value so this can fail
+        && let Ok(one) = ScalarValue::new_one(&sv.data_type())


My only worry here is that we made an effort (I think mostly around stats) to avoid allocating new ScalarValue instances, but this is just a lot nicer.

is the worry about the stack space it takes or the code being invoked to create the instance? curious to learn more about the context here

I went back and found the PRs I remembered:

feat: Reduce allocations for aggregating Statistics #20768

perf: Merge Precision in-place #21219

Seems like the issue was more about aggregating multiple ScalarValue instances, which used to fallback to arrow based aggregations that required heap allocation, so I think this is perfectly fine.

AdamGS

I think a bunch of this code is mine so I figured I'll review it 😅
It all makes sense, just two small comments

Jefffrey added 6 commits July 16, 2026 19:32

Use arrow LUTs for min/max of scalarvals decimals, add dec32+dec64 too

ec65ecb

Since we already have access to these constant lookup tables, its more efficient to index into them rather than calculating via `checked_pow()` at runtime.

Remove support for -1 from unsigned scalar ints

658980d

There's a side-effect of some tests failing so I removed them. I think this is fine since it doesn't make sense to have a `ScalarValue` method where we request a value of a certain type but can get a different value type out.

Add missing null handling arms for arithmetic_negate

b200337

Seems like this was just a simple omission

github-actions Bot added optimizer Optimizer rules common Related to common crate labels Jul 16, 2026

Jefffrey mentioned this pull request Jul 16, 2026

Bump MSRV from 1.88.0 to 1.94.0 #23632

Open

Jefffrey mentioned this pull request Jul 16, 2026

ci: reintroduce code coverage reporting with cargo-llvm-cov #23336

Merged

Jefffrey marked this pull request as ready for review July 16, 2026 11:24

alamb mentioned this pull request Jul 16, 2026

Code coverage report not working #23647

Closed

Merge branch 'main' into fix-scalar-val-num-ops

b2849d9

AdamGS mentioned this pull request Jul 17, 2026

i256 could be more const apache/arrow-rs#10362

Open

AdamGS reviewed Jul 17, 2026

View reviewed changes

AdamGS approved these changes Jul 17, 2026

View reviewed changes

Jefffrey added 2 commits July 17, 2026 22:17

Merge branch 'main' into fix-scalar-val-num-ops

bcc9a35

add TODO

ce0f16b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Various `ScalarValue` numeric method fixes & refactors (especially decimal)#23631

Various `ScalarValue` numeric method fixes & refactors (especially decimal)#23631
Jefffrey wants to merge 9 commits into
apache:mainfrom
Jefffrey:fix-scalar-val-num-ops

Jefffrey commented Jul 16, 2026

Uh oh!

codecov-commenter commented Jul 16, 2026 •

edited

Loading

Uh oh!

Jefffrey commented Jul 16, 2026

Uh oh!

AdamGS Jul 17, 2026

Uh oh!

Jefffrey Jul 17, 2026

Uh oh!

AdamGS Jul 17, 2026

Uh oh!

Jefffrey Jul 17, 2026

Uh oh!

AdamGS Jul 17, 2026 •

edited

Loading

Uh oh!

AdamGS left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Jefffrey commented Jul 16, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jefffrey commented Jul 16, 2026

Uh oh!

AdamGS Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Jefffrey Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

AdamGS Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Jefffrey Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

AdamGS Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamGS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 16, 2026 •

edited

Loading

AdamGS Jul 17, 2026 •

edited

Loading